Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #2488 - Refresh GeoCore Layers on Language Change #2610

Conversation

MatthewMuehlhauserNRCan
Copy link
Member

@MatthewMuehlhauserNRCan MatthewMuehlhauserNRCan commented Nov 21, 2024

Description

Added functionality to the setLanguage function in the map-view to reset the GeoCore layers so they are reloaded in the correct language when available.

Also added a Reset layers check box to the navigator page to test through the UI.

Fixes # 2488

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Tested using the new Checkbox and also in the command line.

Note: Legend order isn't maintained, however; the layer order in the layer tab is.
New Issue: #2611 setMapOrderedLayerInfo isn't applying the visibility to the map

Navigator - GeoCore Config
Navigator - All Layers Config

Checklist:

  • I have build (rush build) and deploy (rush host) my PR
  • I have connected the issues(s) to this PR
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have created new issue(s) related to the outcome of this PR is needed
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

This change is Reviewable

@MatthewMuehlhauserNRCan MatthewMuehlhauserNRCan requested review from jolevesq and DamonU2 and removed request for jolevesq November 21, 2024 16:43
Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @DamonU2 and @MatthewMuehlhauserNRCan)


packages/geoview-core/src/geo/map/map-viewer.ts line 933 at r1 (raw file):

      const promise = AppEventProcessor.setDisplayLanguage(this.mapId, displayLanguage);

      // if flag is true, check if config support the layers change and apply

Wew canc hange the comment as we do not support language in config, just say if flag to reset layer is true


packages/geoview-core/src/geo/map/map-viewer.ts line 936 at r1 (raw file):

      if (resetLayer) {
        const re = /[\w\d]{8}-[\w\d]{4}-[\w\d]{4}-[\w\d]{4}-[\w\d]{12}/;

What is this for? From your comment it is to filter the geoCore layer type. There is a layerType in layerConfig with the geoCore value. You may want to use this instead/


packages/geoview-core/src/ui/style/style.css line 42 at r1 (raw file):

/* smooth data table scrolling */
.MuiTableContainer-root {

Nice add on, the scrolling is much better and faster

Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image.png

vs

image copy 1.png

I think you need to unregister layer from layer set first and then re add them.. The remove layer should do it but seems in problem. Plus i first image, the firts layer has not change...

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @DamonU2 and @MatthewMuehlhauserNRCan)

Copy link
Member Author

@MatthewMuehlhauserNRCan MatthewMuehlhauserNRCan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the Major Projects Inventory layer is working as intended. I don't think there's a translation for it:

Here is the request for that UUID:
https://geocore.api.geo.ca/vcs?lang=fr&id=ea4c0bdb-a63f-49a4-b14a-09c1560aad0b

And this is the WMS that it gives you:
https://openmaps.gov.bc.ca/geo/pub/WHSE_HUMAN_CULTURAL_ECONOMIC.MPI_ECON_MAJOR_PROJECTS_POINT/ows?request=GetCapabilities&service=WMS

And this is the title for the layer:
image.png

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @DamonU2 and @jolevesq)


packages/geoview-core/src/geo/map/map-viewer.ts line 933 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Wew canc hange the comment as we do not support language in config, just say if flag to reset layer is true

Done.


packages/geoview-core/src/geo/map/map-viewer.ts line 936 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

What is this for? From your comment it is to filter the geoCore layer type. There is a layerType in layerConfig with the geoCore value. You may want to use this instead/

That's correct. I'm currently looking in the configs from the "getLayerEntryConfigs()" and I'm not seeing a layerType or any geoCore value. Should I be looking somewhere else?

@MatthewMuehlhauserNRCan
Copy link
Member Author

packages/geoview-core/src/geo/map/map-viewer.ts line 955 at r1 (raw file):

              const uuid = config.geoviewLayerConfig.geoviewLayerId.match(re)![0];
              this.layer.removeLayerUsingPath(config.layerPath);
              return this.layer.addGeoviewLayerByGeoCoreUUID(uuid);

Because I'm adding the layer directly after, could this be causing some of the odd behaviors? Maybe it hasn't fully finished removing the layer and it's already starting the processes to add it back in, which may explain the variety of different possible behaviors? What would be the best way to deal with this? Is there an event or something I can wait for?

Copy link
Member

@DamonU2 DamonU2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jolevesq and @MatthewMuehlhauserNRCan)


packages/geoview-core/src/geo/map/map-viewer.ts line 936 at r1 (raw file):

Previously, MatthewMuehlhauserNRCan wrote…

That's correct. I'm currently looking in the configs from the "getLayerEntryConfigs()" and I'm not seeing a layerType or any geoCore value. Should I be looking somewhere else?

There is a function in the config-api for this: isValidUUID
You can use api.config.isValidUUID(config.geoviewLayerConfig.geoviewLayerId)


packages/geoview-core/src/geo/map/map-viewer.ts line 955 at r1 (raw file):

Previously, MatthewMuehlhauserNRCan wrote…

Because I'm adding the layer directly after, could this be causing some of the odd behaviors? Maybe it hasn't fully finished removing the layer and it's already starting the processes to add it back in, which may explain the variety of different possible behaviors? What would be the best way to deal with this? Is there an event or something I can wait for?

You could test this with a timeout, if it is what's causing issues, you can listen to onLayerRemoved

Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 7 files at r2, all commit messages.
Reviewable status: 7 of 9 files reviewed, 3 unresolved discussions (waiting on @DamonU2 and @MatthewMuehlhauserNRCan)


packages/geoview-core/src/geo/map/map-viewer.ts line 936 at r1 (raw file):

Previously, DamonU2 (Damon Ulmi) wrote…

There is a function in the config-api for this: isValidUUID
You can use api.config.isValidUUID(config.geoviewLayerConfig.geoviewLayerId)

You need to go to parent level to get layerType. entry config is the layer entry and does not have the info

Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r1.
Reviewable status: 7 of 9 files reviewed, 4 unresolved discussions (waiting on @DamonU2 and @MatthewMuehlhauserNRCan)


packages/geoview-core/src/geo/layer/layer.ts line 569 at r2 (raw file):

   */
  refreshGeocoreLayers(): Promise<void> {
    const configs = this.getLayerEntryConfigs();

From the layerEntryConfig you can have access to the parent config who as thelayerType

Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 7 files at r2, 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @DamonU2 and @MatthewMuehlhauserNRCan)


packages/geoview-core/src/geo/map/map-viewer.ts line 933 at r1 (raw file):

Previously, MatthewMuehlhauserNRCan wrote…

Done.

Modify comment for // If flag is true, reload geocore layers


packages/geoview-core/src/geo/layer/layer.ts line 566 at r3 (raw file):

   * @returns {Promise<void>} A promise which resolves when done refreshing
   */
  refreshGeocoreLayers(): Promise<void> {

Would call it reloadGeocoreLayer as it remove and add. We have a refresh function who just reinit states.


packages/geoview-core/src/geo/map/map-viewer.ts line 934 at r3 (raw file):

      // if flag is true, check if config support the layers change and apply
      if (resetLayer) {

Same comment as above for naming. We should call the flag reload

@jolevesq jolevesq requested a review from Alex-NRCan November 28, 2024 13:01
Copy link
Member Author

@MatthewMuehlhauserNRCan MatthewMuehlhauserNRCan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 8 of 11 files reviewed, 5 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @jolevesq)


packages/geoview-core/src/geo/layer/layer.ts line 566 at r3 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Would call it reloadGeocoreLayer as it remove and add. We have a refresh function who just reinit states.

Done.


packages/geoview-core/src/geo/map/map-viewer.ts line 933 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Modify comment for // If flag is true, reload geocore layers

Done.


packages/geoview-core/src/geo/map/map-viewer.ts line 936 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

You need to go to parent level to get layerType. entry config is the layer entry and does not have the info

Done.


packages/geoview-core/src/geo/map/map-viewer.ts line 955 at r1 (raw file):

Previously, DamonU2 (Damon Ulmi) wrote…

You could test this with a timeout, if it is what's causing issues, you can listen to onLayerRemoved

Was an issue with layers not being unregistered from the layer path array in the layerSets. There is a new issue with the visibility not being updated in the map which is likely being caused by the 'MapEventProcessor.setMapOrderedLayerInfo(this.getMapId(), originalMapOrderedLayerInfo);' being called before the layers are fully loaded. There is another issue for this.


packages/geoview-core/src/geo/map/map-viewer.ts line 934 at r3 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Same comment as above for naming. We should call the flag reload

Done.

Copy link
Member

@Alex-NRCan Alex-NRCan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r1, 1 of 7 files at r2, 7 of 8 files at r3, 1 of 3 files at r4, all commit messages.
Reviewable status: 9 of 11 files reviewed, 7 unresolved discussions (waiting on @DamonU2, @jolevesq, and @MatthewMuehlhauserNRCan)


packages/geoview-core/src/geo/layer/layer.ts line 586 at r3 (raw file):

      });

    return Promise.allSettled(promisesOfGeoCoreGeoviewLayers)

When using allSettled() Promise, be mindful that the objects being promised include both PromiseFulfilledResult and PromiseRejectedResult types.

Which means, if you want to map on the results, you shouldn't force cast any promiseobject to a PromiseFulfilledResult<> necessarily. If you only want the promises which fulfilled and ignore the others, I'd suggest that you filter the promises ahead of time.

You may take a look at the other allSettled() call in the same file, in function loadListOfGeoviewLayer where a .filter((promise) => promise.status === 'fulfilled') is made before the map.


packages/geoview-core/src/geo/map/map-viewer.ts line 927 at r3 (raw file):

   * @returns {Promise<[void, void]>}
   */
  setLanguage(displayLanguage: TypeDisplayLanguage, resetLayer?: boolean | false): Promise<[void, void]> {

That comment isn't related to this PR in itself, but I'd suggest adding a TODO to eventually fix this: Promise<[void, void]> which looks like a code smell to me.
Indeed, I fail to see a reason we'd want to carry an array of void elements. Should likely be switched to return a simple Promise inside AppEventProcessor.setDisplayLanguage() instead. It'd fix some odd writing of [undefined, undefined] that you had to do.

Copy link
Member Author

@MatthewMuehlhauserNRCan MatthewMuehlhauserNRCan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 9 of 11 files reviewed, 7 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @jolevesq)


packages/geoview-core/src/geo/layer/layer.ts line 586 at r3 (raw file):

Previously, Alex-NRCan (Alex) wrote…

When using allSettled() Promise, be mindful that the objects being promised include both PromiseFulfilledResult and PromiseRejectedResult types.

Which means, if you want to map on the results, you shouldn't force cast any promiseobject to a PromiseFulfilledResult<> necessarily. If you only want the promises which fulfilled and ignore the others, I'd suggest that you filter the promises ahead of time.

You may take a look at the other allSettled() call in the same file, in function loadListOfGeoviewLayer where a .filter((promise) => promise.status === 'fulfilled') is made before the map.

Added the filter in as well. This was partly due to testing and trying to figure out why the layers weren't being added back in properly. While working on Issue 2611, I can look into simplifying this section as well as looking into your other comment.


packages/geoview-core/src/geo/map/map-viewer.ts line 927 at r3 (raw file):

Previously, Alex-NRCan (Alex) wrote…

That comment isn't related to this PR in itself, but I'd suggest adding a TODO to eventually fix this: Promise<[void, void]> which looks like a code smell to me.
Indeed, I fail to see a reason we'd want to carry an array of void elements. Should likely be switched to return a simple Promise inside AppEventProcessor.setDisplayLanguage() instead. It'd fix some odd writing of [undefined, undefined] that you had to do.

Will look into this while working on Issue 2611

Copy link
Member

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Alex-NRCan and @DamonU2)

@jolevesq jolevesq merged commit fa9ce16 into Canadian-Geospatial-Platform:develop Nov 28, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants